-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve the error handling for HTTP client so consumers can trigger appropriate behavior #793
base: main
Are you sure you want to change the base?
Improve the error handling for HTTP client so consumers can trigger appropriate behavior #793
Conversation
…ppropriate behavior Signed-off-by: Patrick Assuied <[email protected]>
7457f5a
to
18cc895
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like instead of modifying the genericDaprInternalError
class with http properties that are not always relevant, we should extend it to a http specific class, something like:
class DaprHttpError(DaprInternalError):
I like it. What about including the response object in the exception then? I was hesitating about it but didn't want to couple it. |
Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's a great approach @passuied .
I suggested a few changes; I added property decorators on both the new http class and on the existing DaprInternalError class.
This way we can access the properties from the app:
Example:
try:
resp = d.invoke_method(
'invoke-receiver',
'my-method',
data=json.dumps(req_data),
)
except DaprHttpError as e:
print(e.error_code, flush=True)
print(e.status_code, flush=True)
print(e.message, flush=True)
print(e.raw_response_bytes, flush=True)
print(e.reason, flush=True)
print(e.as_dict(), flush=True)
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
Signed-off-by: Patrick Assuied <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing before we merge this. Can we pls add a failure scenario in the examples/invoke-simple
example, that's also used as an e2e test?
You can add something lke this:
try:
resp = d.invoke_method(
'non-existent',
'my-method',
data=json.dumps(req_data),
)
except DaprHttpError as e:
print(e.error_code, flush=True)
print(e.status_code, flush=True)
print(e.message, flush=True)
print(e.raw_response_bytes, flush=True)
print(e.reason, flush=True)
print(e.as_dict(), flush=True)
And then in the README file you can add the expected output around line 48.
Signed-off-by: Patrick Assuied <[email protected]>
Description
Improve the error handling for HTTP client so consumers can trigger appropriate behavior
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #794
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: